Skip to content

fix(e2e): stop wp-env warm-up JSON parse flakes#540

Open
mokagio wants to merge 2 commits into
trunkfrom
mokagio/fix-e2e-wp-env-flaky
Open

fix(e2e): stop wp-env warm-up JSON parse flakes#540
mokagio wants to merge 2 commits into
trunkfrom
mokagio/fix-e2e-wp-env-flaky

Conversation

@mokagio

@mokagio mokagio commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

I run into E2E failures from time to time in this repo over the past few weeks. The last time this happened, in the build for PR #533 which I was reviewing, I decided to task an agent to investigate with /goal E2E green consistently. This is the result.

The approach makes sense to me. While one should avoid polling in tests, I think it's acceptable in the context of E2E . But, I'm not familiar withe the Playground / wp-env system that's in use for the tests, so I can't really judge.

I'm sharing this in the hope it helps getting the E2E back to stable, even if just as a starting point.

AI-generated details below.


What?

Makes the Web E2E job resilient to wp-env warm-up hiccups, which were failing builds intermittently (e.g. build 2401).

Why?

Every failure in the flaky runs traced to wp-env-fixtures.js, where response.json() threw on a non-JSON body returned by the editor-settings/assets REST endpoints during warm-up — either a PHP notice printed into the response (<br /><b>Warning...) or a transient 500. Editor settings are cached in module scope, but only on success, so a single early hiccup poisoned the first fetch and failed every early spec while later ones passed — the classic flaky signature (7 failed / 41 passed with all failures in editor-load, editor-title, editor-error).

How?

Two complementary layers:

  • .wp-env.json: set WP_DEBUG_DISPLAY: false so PHP notices never leak into REST bodies. WP_DEBUG_LOG stays on, so they still land in debug.log.
  • e2e/wp-env-fixtures.js: route both fetches through a fetchJson helper that reads the body as text, retries transient 5xx/parse failures, and includes a body snippet in the final error instead of an opaque Unexpected token '<'.

Testing Instructions

This is a draft to exercise CI — the goal is a green (and repeatably green) Web E2E job.

Locally: make wp-env-start then make test-e2e. Before this change the settings-fetch failures reproduce on a cold instance; after, the suite passes (verified: 47/49 passed, the 2 remaining being local-only media-upload timeouts under fullyParallel workers, which pass at CI's workers: 1).

@mokagio mokagio self-assigned this Jul 6, 2026
@github-actions github-actions Bot added the [Type] Bug An existing feature does not function as intended label Jul 6, 2026
@wpmobilebot

wpmobilebot commented Jul 6, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/540")

Built from 4a07b75

@mokagio mokagio marked this pull request as ready for review July 6, 2026 15:30
Copilot AI review requested due to automatic review settings July 6, 2026 15:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves stability of the Web E2E suite during wp-env warm-up by preventing PHP notices from corrupting REST responses and by making the fixtures/settings fetch more resilient to transient failures.

Changes:

  • Adds a fetchJson helper in e2e/wp-env-fixtures.js that retries transient HTTP/parse failures and surfaces a helpful body snippet on final failure.
  • Extends bin/wp-env-setup.sh to wait until the editor settings endpoint returns valid JSON before proceeding to run tests.
  • Updates .wp-env.json to disable WP_DEBUG_DISPLAY so PHP notices don’t leak into REST response bodies.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
e2e/wp-env-fixtures.js Routes editor settings/assets fetches through a retrying JSON fetch helper to reduce warm-up flakes.
bin/wp-env-setup.sh Adds a readiness gate for the editor settings REST endpoint to avoid early warm-up failures.
.wp-env.json Disables debug display to keep REST responses clean while preserving debug logging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mokagio mokagio requested a review from jkmassel July 7, 2026 09:07
@iangmaia

iangmaia commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@claude review this PR.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @iangmaia's task in 0s —— View job


I'll analyze this and get back to you.

@iangmaia

iangmaia commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @iangmaia's task in 0s —— View job

I'll analyze this and get back to you.

FYI #533 isn't merged so ofc this won't trigger the new Claude workflow.

@mokagio

mokagio commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@claude review this PR, now that #533 landed and the workflow is secured.

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @mokagio's task in 0s —— View job


I'll analyze this and get back to you.

@mokagio

mokagio commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@iangmaia looks like the Claude review run but never went past reporting that it started? I had a quick look at the logs, https://github.com/wordpress-mobile/GutenbergKit/actions/runs/28994702797/job/86041799638, but couldn't make much of it. Haven't passed it to my AI for further investigation, but can do that later if you don't have the bandwidth.

mokagio and others added 2 commits July 15, 2026 21:11
E2E runs failed intermittently when the wp-env REST endpoints returned a
non-JSON body during warm-up — a PHP notice printed into the response
(`<br /><b>Warning...`) or a transient `500` — and `response.json()` threw.
Because editor settings are cached in module scope only on success, a single
early hiccup poisoned the first fetch and failed every early spec while later
ones passed: the flaky signature.

Two complementary fixes:

- Set `WP_DEBUG_DISPLAY: false` so PHP notices never print into REST bodies.
  `WP_DEBUG_LOG` stays on, so notices still land in `debug.log`.
- Route the settings and assets fetches through a `fetchJson` helper that
  reads the body as text, retries transient `5xx`/parse failures, and surfaces
  a body snippet in the final error instead of an opaque `Unexpected token '<'`.

---

Generated with the help of Claude Code, https://claude.com/claude-code

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI surfaced the real warm-up failure the earlier retry only papered over: the
Playground runtime keeps installing plugins (Blueprint steps) after it starts
serving requests. During those installs WordPress drops a `.maintenance` file
and returns a `503`, and there is a race where the file is removed mid-request
and PHP fatals — both corrupt the first editor REST call.

Gate provisioning in `wp-env-setup.sh` on the settings endpoint returning valid
JSON, so the suite never starts against a half-provisioned instance. Raise the
in-test fetch retry budget (5s -> 30s) as a backstop for any mid-run blip.

---

Generated with the help of Claude Code, https://claude.com/claude-code

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@iangmaia iangmaia force-pushed the mokagio/fix-e2e-wp-env-flaky branch from 7f06856 to 4a07b75 Compare July 15, 2026 19:11
@iangmaia

iangmaia commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

@iangmaia looks like the Claude review run but never went past reporting that it started? I had a quick look at the logs, https://github.com/wordpress-mobile/GutenbergKit/actions/runs/28994702797/job/86041799638, but couldn't make much of it. Haven't passed it to my AI for further investigation, but can do that later if you don't have the bandwidth.

It seems to be a problem with the token, which doesn't surface properly. See anthropics/claude-code-action#1249.
I'll create a new token and update it in the action secrets.

@iangmaia

Copy link
Copy Markdown
Contributor

@claude review this PR.

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @iangmaia's task in 1m 16s —— View job


PR Review: fix(e2e): stop wp-env warm-up JSON parse flakes

  • Read changed files (.wp-env.json, bin/wp-env-setup.sh, e2e/wp-env-fixtures.js)
  • Analyze diff vs origin/trunk
  • Provide review feedback

Overall this is a solid, well-reasoned fix. The root-cause diagnosis (PHP notices / transient 5xx corrupting the first REST body, poisoning module-scoped caching only-on-success) is convincing, and the three layers address it at the right places. The code is clean, the JSDoc is genuinely helpful, and the error messages now surface a body snippet instead of the opaque Unexpected token '<'. Nice work. A few thoughts below — none are blockers.

🟡 fetchJson retries every error, not just transient ones

e2e/wp-env-fixtures.js:78-83 — the catch treats all failures identically, so a genuinely permanent error (e.g. 401/403 from a bad authHeader, or 404 from a wrong route) is retried the full 15× 2s = 30 s before failing. The JSDoc and PR description both say "retry transient 5xx/parse failures," so the code is slightly broader than advertised.

This isn't wrong for CI resilience, but failing fast on non-retryable status codes would make a real misconfiguration surface in seconds instead of after a 30 s hang, and would match the stated intent:

if ( ! response.ok ) {
    const message = `HTTP ${ response.status } ${ response.statusText }${ body.slice( 0, 200 ) }`;
    // 4xx (except 408/429) won't fix themselves on retry — fail fast.
    if ( response.status < 500 && response.status !== 408 && response.status !== 429 ) {
        throw new Error( `Failed to fetch ${ label }: ${ message }` );
    }
    throw new Error( message );
}

(A network-level fetch() rejection and a JSON parse failure should still retry, since those are the transient warm-up cases.) Fix this →

🟢 Redundant readiness gating (by design, worth a note)

bin/wp-env-setup.sh:183-208 gates on the settings endpoint returning valid JSON, and fetchJson then retries the same endpoint again. That's intentional belt-and-suspenders and I think it's justified for a flaky suite — but note the setup gate only covers wp-block-editor/v1/settings, not the wpcom/v2/editor-assets endpoint, so the in-test retry still carries real weight for assets. No change needed; just flagging that the two layers aren't fully redundant.

🟢 Minor: duplicated inline Node JSON-parse snippet

bin/wp-env-setup.sh now embeds the same process.stdin … JSON.parse node snippet twice (app-password extraction at ~L128 and the readiness gate at ~L190). Not worth a helper for two occurrences, but if a third appears it'd be worth extracting.

🟢 Nits

  • e2e/wp-env-fixtures.js:87lastError.message is safe here since MAX_FETCH_ATTEMPTS is 15 (the loop always runs at least once and assigns lastError before this line). Fine as-is; just noting the invariant isn't obvious.
  • The 200-char body snippet in error messages could echo part of the editor-settings payload into CI logs. For local/CI test infra against localhost this is a non-issue, but worth keeping in mind if these fixtures are ever pointed at a shared instance.

Testing & standards

  • .wp-env.jsonWP_DEBUG_DISPLAY: false while keeping WP_DEBUG_LOG: true is exactly the right call: notices stay in debug.log but never leak into REST bodies. 👍
  • The polling-in-tests tradeoff the author raised is reasonable for E2E warm-up; bounded retries with a hard cap and a diagnostic final error is the acceptable form of it.

Great targeted fix — my only real suggestion is the fast-fail-on-4xx tweak; everything else is optional polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants